Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [ISSUE-1401]: Added helm get values option in Helm collector #1402

Merged
merged 4 commits into from
Jan 16, 2024

Conversation

avaakash
Copy link
Contributor

@avaakash avaakash commented Dec 16, 2023

Description, Motivation and Context

This PR adds the helm values collection enhancement to Helm collector.
For more information, refer to this issue: #1401

Checklist

  • New and existing tests pass locally with introduced changes.
  • Tests for the changes have been added (for bug fixes / features)
  • The commit message(s) are informative and highlight any breaking changes
  • Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

  • Yes
  • No

@avaakash avaakash requested a review from a team as a code owner December 16, 2023 03:10
@CLAassistant
Copy link

CLAassistant commented Dec 16, 2023

CLA assistant check
All committers have signed the CLA.

@avaakash avaakash marked this pull request as draft December 16, 2023 03:10
@banjoh banjoh added the type::feature New feature or request label Dec 18, 2023
Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution!

The overall change looks good to me. I have added some comments, mostly around cleanup and some improvements I noticed that need to be done

The other missing bit that would be needed in this PR is

pkg/collect/helm.go Show resolved Hide resolved
pkg/collect/helm.go Outdated Show resolved Hide resolved
pkg/collect/helm.go Outdated Show resolved Hide resolved
pkg/collect/helm.go Outdated Show resolved Hide resolved
Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error handling changes look good, thanks for that. I have added one code suggestion to the PR

pkg/collect/helm.go Outdated Show resolved Hide resolved
@avaakash avaakash marked this pull request as ready for review January 11, 2024 21:11
@banjoh
Copy link
Member

banjoh commented Jan 12, 2024

Docs PR: replicatedhq/troubleshoot.sh#534

Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR changes work. I added one last comment needing to be changes

test/e2e/support-bundle/helm_collector_e2e_test.go Outdated Show resolved Hide resolved
pkg/collect/helm.go Outdated Show resolved Hide resolved
Signed-off-by: Akash Shrivastava <[email protected]>
Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@banjoh banjoh merged commit 361e12e into replicatedhq:main Jan 16, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type::feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants